grpc/client: Implement Channel builder#2675
Conversation
1014b27 to
821aa43
Compare
Make Channel::builder public and update ChannelBuilder::credentials to accept impl IntoDynChannelCredentials instead of Arc<dyn DynChannelCredentials>.
821aa43 to
c0e1055
Compare
…nChannelCredential impl adjustments should handle the type issue when passing DynChannelCredentials internally
|
As discussed offline, I've raised #2694 with only the changes to the trait bounds for the credentials. I'll wait for Doug to review it. |
|
Update: #2703 has been merged to make the |
a8e3dac to
e197877
Compare
arjan-bal
left a comment
There was a problem hiding this comment.
I have left some new comments, and some of the previous feedback still needs to be addressed. Please take a look.
…er from PresentOpt since it is unneeded.
4288b7f to
8c1e34b
Compare
| // of satisfying the credential/security configuration through different means | ||
| // in the future (via adding methods to this impl taking different args). | ||
| impl<Runtime> ChannelBuilder<MissingOpt, Runtime> { | ||
| pub fn credentials( |
There was a problem hiding this comment.
Please add rustdoc for public methods. I would also recommend using documentation tests to demonstrate the usage.
There was a problem hiding this comment.
(Still working on this.)
There was a problem hiding this comment.
I believe all the doc tests are updated with this, and the public functions in channel.rs all have documentation now.
664e2cc to
fbcbbb1
Compare
| #[cfg(feature = "_runtime-tokio")] | ||
| use crate::rt::default_runtime; |
There was a problem hiding this comment.
We can remove the default_runtime function, the NoOpRuntime and directly call TokioRuntime::default() in the Channel builder constructor since it's already guarded by a feature flag now.
There was a problem hiding this comment.
I would like to do this in a follow-up. Right now we're using it over sixteen files so I'd rather those not pollute this change with that, if that's alright?
| } | ||
| } | ||
|
|
||
| pub fn channel_authority(mut self, authority: impl Into<String>) -> Self { |
There was a problem hiding this comment.
We should document this. You can take inspiration from Go's dial option: https://pkg.go.dev/google.golang.org/grpc#WithAuthority
There was a problem hiding this comment.
Done! LMK if the language is clear enough.
Introduces a type-builder Channel builder for creating new Channels, using an idiomatic pattern for adding in configuration options.
Motivation
In order to flexibly and ergonomically provide for channel configuration on construction, the type builder is introduced. This allows users to construct channels in a manner similar to:
Solution
The solution uses a 'type builder' so that users can build up their channel configuration over various calls, and the compiler can catch type issues. This also allows us to encapsulate various ergonomics (such as default selection of runtime). This PR implements the critical values at this time and defers additional optional values not being used until a later point. It also leaves work around the internals of the
PersistentChannel/ActiveChannelfor a future PR.Because the public API is being altered, this requires a version bump.